-
-
Notifications
You must be signed in to change notification settings - Fork 833
ParameterList - Cache the size and avoid iterator pattern #1848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I have been working on a very large monolith using Hibernate ORM and these particular methods were quite high in the profiling I made. Using this simple trick reduces things quite a lot, for what I think is an acceptable trade-off of the code looking a bit old school. FWIW, we have been fighting against iterator() in Quarkus in critical hot paths as it generates more allocations than simply going through the list. What is specific here is that size() is also quite slow in some cases, and it's also called by hasNext().
|
👋 @raphw not sure if you remember me, we interacted a bit a few years back when you rewrote the Hibernate enhancer to ByteBuddy. Let me know what you think of this patch, happy to adjust if needed. |
|
FWIW, if you want to reproduce, I'm using a benchmark I can share, it's just a bunch of generated entities in a Quarkus application. I have scripts there to get CPU profiles and allocations profiles. |
|
Of course I remember! I am curious: is it necessary to both avoid the iterator, and to avoid the call to "size"? I would expect iterators to normally be optimized by the JIT if they lie ion a critical code path. Generally speaking, I am happy to optimize here. |
|
So, the When we have a lot of entities, allocations are really in the way. FYI, on my benchmarks with this generated application with 4000 entities, the Hibernate ORM enhancement + proxy generation with ByteBuddy is taking 40% of the CPU and allocations of the build. This change only helps a bit and it was an easy one. For the rest, things are a lot more complex and I haven't yet figured out how to improve things in a meaningful way. |
|
If you measured, I am happy. I still made some changes, for example computing the size lazily. Could you check that the performance improvements are retained by this change? |
|
I'll have a look tomorrow! |
|
@raphw from what I can see, things look fine after your patch. Thanks. I had another small question: if I have a Basically, my question is that we are doing some operations in two steps and I would like to know if I can reuse the type pool from the first steps (I need to make sure the changes mades at the previous step are visible to the second step). That would be my expectation but I want to make sure it's true. |
|
I refactored a little more as the initialization value of the field would be As for the |
I have been working on a very large monolith using Hibernate ORM and these particular methods were quite high in the profiling I made.
Using this simple trick reduces things quite a lot, for what I think is an acceptable trade-off of the code looking a bit old school.
FWIW, we have been fighting against iterator() in Quarkus in critical hot paths as it generates more allocations than simply going through the list.
What is specific here is that size() is also quite slow in some cases, and it's also called by hasNext().
This is before:
Zoom on the interesting part:
This is after:
Zoom on the interesting part: